Convert tests/debuginfo/pretty-std.rs to lldb-repr - #160331

jieyouxu is currently at their maximum review capacity.
They may take a while to respond.

Damn. This PR makes me quite concerned about the approach we took with the JSON files. This is a single ported test, which results in a ~3k diff. I knew it would be bad, but not this bad. And this is still only for LLDB, not even with GDB included! That means that every ported test, but also every new added debuginfo test, would likely add a massive JSON file. That's not great.

Another thing that I don't like here is how to review the JSON files. The thing that I want to check the most is the user-facing output for the individual debugged variables, primarily their pretty print and the rendering of their children. This data is currently buried within ~1k lines of JSON that contains a lot of other information, and it is difficult for me to find the interesting data within all that noise.

I yet again wonder if we are optimizing for the right thing here. I know that you want to see all these details in case something breaks and you need to debug it, and that you also want to see historical changes of these details, in case something in them changes without the pretty print/children changing at the same time. But it still feels to me like we are essentially committing compiler debug logs and Debug prints of internal compiler data to git for each test, so that we can check historically how they changed. We don't do this for pretty much any other test suite, and I don't think that we should do it here.

The common case is the user observable parts (pretty print/children) breaking, and I think that is the thing that we should focus on, same as with other UI tests.

I think that there is a compromise that we could make to support your use-case of going back in time and checking when a given internal representation changed, while making the committed files be much smaller and making it easier to review them.

• By default, we would commit only a much simpler JSON file with basic information about the debugged variables, and the debugger metadata used to create the file, and nothing else.
• We would implement a simple cargo-rustc-bisect wrapper, which would run a given debuginfo test starting from some past version, generate the complex JSON (that is committed now) for each version, and display the diffs. This would allow going back in time and checking when a given internal representation might have changed. Note that only a single LLDB version would be required for this, so the bisection should be rather simple. Because LLDB bumps would anyway invalidate the JSON's internal representation. But if needed, support for multiple LLDB version could be added.

What do you think? :)

I'm not particularly happy about the file size either. There are ways we can reduce it (e.g. collapsing "array-like" children into a flat array instead of full child listings, consolidating generics into a single type listing, etc.). Lots of the existing tests cover redundant things, so they can (and probably should) be consolidated into a smaller number of longer tests, which amortizes the type data. We can also use an alternative data format, we'd just need to swap out the ser/de code. That kind of space optimization seems beside the point at the moment though, as you're suggesting we don't store the type data at all.

It is my understanding that the old test logic never had issues detecting user-facing regressions, so long as the tests were actually enabled. Rather, the problem was that people would disable the tests instead of fixing them because the effort and time to learn enough to fix them, and then actually fix them, was not deemed worthwhile. Storing the type information intends to solve that problem.

To be entirely clear, I do not personally need the type information stored. It saves me a bunch of time, but I don't need it. I am relatively familiar with the debug info we generate, how it appears in the DWARF/PDB data, and how the debuggers read, represent, and expose it. The same cannot be said for almost any other contributor to the rust repo. There's just too much surface area for everyone (or even most people) to be familiar with debug info. Someone could change the heuristic for niche optimization and break visualizers. Someone could (and has, multiple times), changed the field names of Vec and broken the visualizers¹. Debug info is pervasive and unavoidable. It is impossible to keep these breakages away from people who aren't familiar with debug info. The type-data storage and error messages are largely to prevent those cases from turning into "fuck it, just disable the tests", or people just updating it in-place and causing a regression because the contributor and reviewer both don't know enough to spot the regression.

The intent is to not have to inspect the json data except in extreme circumstances (and/or in diffs of ~a few lines for patches and such). The data feeds the error messages that describe the issue in enough detail that people don't have to look at the data. I understand that the initial diff is rough, but that's a 1-time problem when the test is first created.

The thing that I want to check the most is the user-facing output for the individual debugged variables, primarily their pretty print and the rendering of their children. This data is currently buried within ~1k lines of JSON that contains a lot of other information, and it is difficult for me to find the interesting data within all that noise...But it still feels to me like we are essentially committing compiler debug logs and Debug prints of internal compiler data to git for each test, so that we can check historically how they changed. We don't do this for pretty much any other test suite, and I don't think that we should do it here.

I still disagree with the notion that anything not user-facing is "noise" in the context of these tests. The type information in the json file is not rustc's type information. It is the debugger's interpretation of the debug info rustc asked LLVM for, through the lens of LLVM lossily translating it to-and-from 2 file formats we don't control.

The visualizers are ~100% load-bearing assumptions based data we receive from a massive black box that can do literally anything at all before it gives it back to us. For example, LLDB ignores the primitive type names we ask LLVM for and uses C-style names. GDB ignores pointer-type names and makes them all *mut. LLDBs truncate every enum discriminant to 32 bits. LLDB could trivially replace every type name with aaaaa, every value with 12345, add arbitrary fake fields to everything, lie about size and alignment, and only allow emojis when indexing Vec, and no changes to rustc's debug info generation would fix that.

A TypeSystem is supplied a debug info file, but it doesn't have to use it or respect any of the information in it. It's simply asked "get me the thing with this name" or "get me the fields of this type" and can provide literally whatever it wants to. GDB has similar capabilities from what I've seen.

I don't believe it makes sense to ignore the fact that we are operating on completely untrusted data. The tests disabled with the message:

// LLDB 1800+ tests were not tested in CI, broke, and now are disabled

Attest to that. I haven't personally checked all of them, but it's highly likely that the data LLDB provided changed in version 18+ (despite no changes occurring on our end). That broke our assumptions, but those assumptions were completely undocumented and we did not test that they were upheld. The failure happens too late in the pipeline, making the source of the failure unclear. That resulted in the tests being disabled instead of fixed.

I don't think testing untrusted data is unique in rust's test suites. tests/codegen_llvm seems to be exclusively filled with "is <the black box of LLVM> giving us the IR/ASM output we expect with the way we are asking for it?". There are even debuginfo codegen tests there that check that we're getting the correct debug info IR nodes (the reason those checks are semi-irrelevant to the visualizers is because, as stated earlier, LLVM translates these nodes lossily to DWARF and PDB, and the debuggers don't have to respect what's in those files).

Imo, not testing the data we receive from lldb/gdb would be akin to rm rf ./tests/codegen_llvm because when we compile println!("{:?}", vec![1, 2, 3) it prints [1, 2, 3]

It is my understanding that the old test logic never had issues detecting user-facing regressions, so long as the tests were actually enabled. Rather, the problem was that people would disable the tests instead of fixing them because the effort and time to learn enough to fix them, and then actually fix them, was not deemed worthwhile. Storing the type information intends to solve that problem.

I can't 100% say what was the main issue in general, but for me, the biggest practical issue was simply the lack of blessing, which we do have now. We are also now ready for pinning the debugger version in CI, and will support only one version. There are still things to figure out (how to bless across OSes, and how to provide people a way to download the "right" debugger from CI), but I think that things are already in a better shape than they used to be. I don't think that having 3k lines of JSON for each test is what keeps us from having maintainable debuginfo tests (though it can likely help in some situations, I'm not disputing that. I'm just wondering whether the cost-benefit ratio here).

The intent is to not have to inspect the json data except in extreme circumstances

That's IMO exactly why storing them in git and putting them into everyone's faces might not be the best trade-off to make.

Originally I let us keep the large JSON files because I thought that you might use them when debugging (pun intended) the debuginfo tests, but now you're saying that you actually don't need them. I know that you want those files to be useful to others, but I'm afraid that the chance of pretty much anyone else than you making use of all the extended details stored in those JSON files to debug something is very small, because most people won't have the knowledge required to interpret them.

What do you think about the option of backfilling the extended data in the rare cases where we actually need to go back and debug? It shouldn't be that difficult.

That being said, I think that there are also some other compromises that we could make to reduce the file size, and maybe then it wouldn't be that bad:

• Skip writing values that are null.
• Skip writing nested children fields that have value [].
• Try to coerce the JSON writer to try to write children on a single line (but that might be difficult to do...).
• Remove the types field from the JSON files. And instead, have a single test that will contain all the interesting stdlib types that we might want to print. If there is a new test for which we want to use a new stdlib type, we add it to this file. Then, this test would use a different debugger command (something like repr-type), which would print the type information, or we just use repr, and repr will print both the value and the type (but this would again make all other tests' JSON files larger). For non-std files, we could use repr-type for them by default, to include their (non-recursive) type in the JSON file.
• Ditch JSON altogether, and instead re-render the original input source file (without compiletest comments), with the debuginfo information embedded on top of the variables being repr-printed. This could reduce the size, and also make it easier to inspect the result. In theory, we could let compiletest bless the output directly in-place in the original test, but I feel like that would become too fragile and hard to read. Something like:

fn main() {
    // lldb: repr vec
    let vec = vec![4u64, 5, 6, 7];
}

turns into a blessed file (one per OS/target):

fn main() {
    // type: alloc::vec::Vec<unsigned long long, alloc::alloc::Global>
    // pretty_print: size=4
    // synthetic: lldb_lookup.StdVecSyntheticProvider
    // summary: lldb_lookup.SizeSummaryProvider
    // children:
    // [0], type: unsigned long long, value: 4
    // [1], type: unsigned long long, value: 5
    // [2], type: unsigned long long, value: 6
    // [3], type: unsigned long long, value: 7
    let vec = vec![4u64, 5, 6, 7];
}

I don't think that having 3k lines of JSON for each test is what keeps us from having maintainable debuginfo tests

"Maintainability" to me also implies that problems are clearly sourced and reasonable to fix for those who encounter them. That has clearly not been the case thus far. Very few people have been willing to work on debug info. Lack of debugger expertise has been sited by multiple people as part of why things got as bad as they did. I wrote docs to help onboard people, but I can't force people to read them. Encoding my knowledge of the debuggers in the tests (in the form of typical diagnostic steps and inspecting the common causes of errors) is the next best thing I can do, and that requires the underlying data that caused the error.

That's IMO exactly why storing them in git and putting them into everyone's faces might not be the best trade-off to make.

I'm not sure I understand how this is "putting them into everyone's faces". The json lives in a subdirectory, the error reporting does not output the raw json, the git diff on a typical change will be ~1-5 lines of the json that are the direct input and output of the change.

Originally I let us keep the large JSON files because I thought that you might use them when debugging (pun intended) the debuginfo tests, but now you're saying that you actually don't need them

When i say "need", i mean it in the sense that it is literally possible for me to diagnose issues without them, just as i'm sure it's literally possible for someone familiar with aliasing rules and compiler optimizations to diagnose pointer UB by hand rather than using MIRI or whatever. Do I actually want to, in the long-term, spend my time tediously verifying a bunch of simple stuff by hand every time a test fails, just to even know what kind of failure i'm working with? No, not really.

Regardless, I am not the only one who needs to diagnose tests/debuginfo test failures. Can we reasonably expect that the majority of those who could encounter a failure would know what steps to take to diagnose the issue? Based on the state of the test suite, probably not.

I'm afraid that the chance of pretty much anyone else than you making use of all the extended details stored in those JSON files to debug something is very small, because most people won't have the knowledge required to interpret them.

They don't have to interpret the JSON though. check_lldb.py interprets it for them and outputs the interpretation as error messages that explain exactly what changed, whether or not it broke the visualizers, and (where possible) what the source of the problem is. We would not be able to offer comprehensive error messages without the data existing somewhere to compare against.

LLDB's own tests verify the output of DWARFASTParserClang/PDBASTParserClang (which are underpinned by LLVM's DWARF/PDB decoding) and the output of TypeSystemClang (i.e. LLDB's interpretation of the type layout) as part of its tests. As part of the LLVM repo, they can also rely on clang and LLVM's tests to validate their input data. We don't have that luxury, and we're also making use of TypeSystemClang in a hack-y way by sortof pretending to be C++.

If we continue to treat visualizers as if they work by magic, that we have no idea how or why they fail, every failure will continue to feel "random", and people will continue to struggle to fix them.

I am sympathetic to the issue of json size, and am open to finding ways to make the output smaller, but i don't think "blindly trust data from an outside source that we KNOW changes and breaks our output" would be considered acceptable in any other test suite.

Remove the types field from the JSON files. And instead, have a single test that will contain all the interesting stdlib types that we might want to print. If there is a new test for which we want to use a new stdlib type, we add it to this file.

Back in January I mentioned that we can condense a lot of the existing tests into ~1 test per visualizer. Would that be preferable instead of a single file that handles all the visualizers? There's a lot of other condensing we can do too. Lots of the current tests are redundant and/or are target/debugger-specific versions of things (e.g. tests/debuginfo/gdb-pretty-struct-and-enums.rs, tests/debuginfo/msvc-pretty-enums.rs).

instead re-render the original input source file (without compiletest comments), with the debuginfo information embedded on top of the variables being repr-printed.

This seems like it would get pretty messy once we have output for more than 1 debugger. Having to handle intermixed test code and per-debugger data during serialization and de-serialization also seems error-prone.

I added an experimental commit with some space-saving measures (only blessed on non-windows atm). In terms of "non-destructive" changes

• BasicType.Invalid is replaced with None
• All None values, empty lists, and empty dicts are removed just before saving (the way this is implemented would break multi-breakpoint tests IIRC, but for this proof-of-concept it's fine)
• An alternative child format is used for "array-like" containers, which stores a single type entry that applies to all of the children. The child-names can also be omitted since our visualizers output them as [<idx>]
• Variable data is placed before type data in the json file

There's also 1 "semi-destructive" change. Essentially it doesn't output type information if the source variable doesn't have a synthetic, summary. This is based on the assumption that we don't make assumptions about the data LLDB provides if we don't have a visualizer. I call it "semi-destructive" (relative to the output without this change) because that assumption isn't really true, so we do end up not verifying things that we were verifying before.

This change is not observable in the pretty-std test because we cannot cull inner types without knowing exactly what parts of each struct the visualizers use. I did re-bless basic-types though, and the change can be observed there. Note that we are no longer verifying lldb.eBasicType nor the size of the type in that test though.

This seems like it would get pretty messy once we have output for more than 1 debugger. Having to handle intermixed test code and per-debugger data during serialization and de-serialization also seems error-prone.

We would still have a separate file per debugger. And there is no serialization/deserialization that has to happen, the test harness takes the test, interprets the magic commands, generates the golden file and if it doesn't match with what is committed, then it's a test failure. And blessing would just overwrite the golden file.

Regarding the "in everyone's faces" thing, I mostly meant having a several thousand line diff for every added debuginfo test, that is IMO not ideal for reviewing.

Thank you! That already looks much better, with the implicitly default values being ignored. I also like that in the pretty-std test, the things we care about the most (the debugged values) are now stored first, before the types.

What do you think about my earlier suggestion to reduce the shown type information? Either:

• Only print the types of the variables being repr-printed, and not all "mentioned" types (I don't actually know which types are printed, like what's the logic for deciding them, haven't checked that code in detail yet)
• Have a separate test that will print interesting types, with a repr-type command, or something similar, and not print types implicitly in other tests.

By the way, I hope I'm not coming across as being too negative, I know that it's not a great feeling when the review bickers so much. I appreciate all the thoughts and work you put into this, and I don't want to block it, but I also want to ensure that we at least try to make the JSON files smaller.